Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New implementation based on julia-0.5 infrastructure #2

Merged
merged 1 commit into from
Jun 16, 2016
Merged

New implementation based on julia-0.5 infrastructure #2

merged 1 commit into from
Jun 16, 2016

Conversation

timholy
Copy link
Member

@timholy timholy commented Jun 14, 2016 •
edited
Loading

JuliaLang/julia#16260 added support for arrays with unconventional indexing. This essentially rewrites this package based on that infrastructure. It seems to be pretty compatible with your implementation; of the examples, only "main_sub" fails, and as far as I can tell this implementation doesn't actually use this package (it uses tricks with SubArray, but on julia-0.5 those tricks won't work anyway because bounds-checking has been introduced into SubArrays). You could presumably rewrite that example?

I noticed that despite the fact that this is a registered package, there is no tagged version. That means users can't Pkg.add("OffsetArrays"). Given the requirement for julia-0.5 here, my recommendation is the following:

  • first, before doing anything else you tag an official version of this this packages as it is now, before merging this. It could be tagged something like v0.1.0 (Pkg.tag("OffsetArrays", :minor); Pkg.publish(). You should do that even if you have no intention of merging this.
  • create and push a new branch, called pre-arraymageddon or something. This is for allowing you to update the old code in case you discover bugs for prior Julia versions.
  • then, if/when you're comfortable with this implementation, you can merge this
  • tag a version that's a minor-version bump (e.g., v"0.2.0") for the new implementation.

Users not running julia-0.5 will get the old one, and the fact that there's a minor version bump means that you can continue to release new versions for older versions of Julia, basing the changes off the pre-arraymageddon branch.

@timholy
Copy link
Member Author

timholy commented Jun 14, 2016

Now that there are tests, you also might want to consider visiting travis-ci.org and turning on Travis testing.

@alsam
Copy link
Member

alsam commented Jun 14, 2016

@timholy thanks a lot for excellent job and for detailed merging instructions!
I will look into it soon.

@alsam alsam merged commit 717b881 into JuliaArrays:master Jun 16, 2016
@alsam
Copy link
Member

alsam commented Jun 16, 2016 •
edited
Loading

@timholy thanks again. I merged your changes.
Tried to to tag, but unfortunately get

julia> Pkg.tag("OffsetArrays", :minor)
ERROR: Pkg.tag(pkg, [ver, [commit]]) has been moved to the package PkgDev.jl.
Run Pkg.add("PkgDev") to install PkgDev on Julia v0.5-
 in tag(::String, ::Symbol) at ./pkg/pkg.jl:263
 in eval(::Module, ::Any) at ./boot.jl:231
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

despite the fact I followed the above recommendations and made

julia> Pkg.add("PkgDev")
INFO: Cloning cache of PkgDev from https://github.com/JuliaLang/PkgDev.jl.git
INFO: Installing PkgDev v0.1.0
INFO: Package database updated

the error message for ERROR: Pkg.tag stuck.
So unfortunately the package left untagged.
Will look into it later.

The actual error message:

julia> PkgDev.tag("OffsetArrays", v"0.2.0");
ERROR: OffsetArrays is not a git repo
 in tag(::String, ::VersionNumber, ::Bool, ::String) at $HOME/.julia/v0.5/PkgDev/src/entry.jl:205
 in (::Base.Pkg.Dir.##2#3{Array{Any,1},PkgDev.Entry.#tag,Tuple{String,VersionNumber,Bool}})() at ./pkg/dir.jl:31
 in cd(::Base.Pkg.Dir.##2#3{Array{Any,1},PkgDev.Entry.#tag,Tuple{String,VersionNumber,Bool}}, ::String) at ./file.jl:59
 in #cd#1(::Array{Any,1}, ::Function, ::Function, ::String, ::Vararg{Any,N}) at ./pkg/dir.jl:31
 in tag(::String, ::VersionNumber) at $HOME/.julia/v0.5/PkgDev/src/PkgDev.jl:40
 in eval(::Module, ::Any) at ./boot.jl:231
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

looking into it.

@alsam
Copy link
Member

alsam commented Jun 16, 2016

Ok, looks like figured out what went wrong.
Going to do the following: revert @timholy changes, tag correctly, publish, after that merge the changes again with the correct tag for Julia 0.5.

@alsam alsam mentioned this pull request Jun 18, 2016
Closed
@timholy
Copy link
Member Author

timholy commented Jun 19, 2016

Sorry I missed this conversation (buried below some other emails), but glad you figured it out!

@alsam
Copy link
Member

alsam commented Jun 19, 2016

Sure no problem, and thanks again for your contribution!

I've already tagged with v0.1.3 the old pre-0.5 code. I put it to pre-arraymageddon branch, as you proposed and after hat pushed your code to master.
However something is broken with the new PkgDev package:
tagged it ok with

julia> PkgDev.tag("OffsetArrays", v"0.2.0");
INFO: Tagging OffsetArrays v0.2.0
INFO: Committing METADATA for OffsetArrays

but wasn't able to publish the changes:

julia> PkgDev.publish()
...
INFO: Validating METADATA
INFO: Pushing OffsetArrays permanent tags: v0.2.0
ERROR: MethodError: no method matching Base.LibGit2.UserPasswordCredentials(::Void, ::String)
Closest candidates are:
  Base.LibGit2.UserPasswordCredentials(::AbstractString, ::AbstractString)
  Base.LibGit2.UserPasswordCredentials{T}(::Any)
 in credentials() at .../.julia/v0.5/PkgDev/src/github.jl:126
 in (::PkgDev.Entry.##6#9{Dict{String,Array{String,1}}})(::Base.LibGit2.GitRepo) at .../.julia/v0.5/PkgDev/src/entry.jl:99
 in with(::PkgDev.Entry.##6#9{Dict{String,Array{String,1}}}, ::Base.LibGit2.GitRepo) at ./libgit2/types.jl:660
 in publish(::String, ::String) at .../.julia/v0.5/PkgDev/src/entry.jl:84
 in publish() at .../.julia/v0.5/PkgDev/src/PkgDev.jl:55
 in eval(::Module, ::Any) at ./boot.jl:231
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

@timholy timholy deleted the pull-request/717b881b branch June 19, 2016 23:16
@timholy
Copy link
Member Author

timholy commented Jun 19, 2016

Yep, that's perhaps the single biggest release blocker for julia-0.5.

@alsam
Copy link
Member

alsam commented Jun 20, 2016

I see, I filed an issue for PkgDev
JuliaLang/PkgDev.jl#48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timholy @alsam